Repository navigation
Various fixes to allow the daily update-driver-submodules workflow to complete. - #35
Conversation
7a27489 to
036bdd0
Compare
|
In the resulting drivers README, the links of the alt drivers are missing the "-alt" suffix in the name of the repo, leading to a "Page not found". |
|
Not modified by the PR, but you might want to look for :
Missmatch write a file with
|
27f896f to
9d810e2
Compare
|
Thanks for your good catches guys! I've (presumably) fixed them with the few extra commits on the branch. Sadly I cannot do a full test-run now that I'm using the PAT secret because it is not passed down to workflow, but I've run it locally and can confirm it fixed the -alt URL. I'll retest after merging and address any newly found issue in a new PR if that's OK, unless you can already see some issues with my iterative fixes. |
…ules" The artipacked finding is fixed properly upstream in xcp-ng#35, which sets persist-credentials explicitly on the checkout (what actually silences the audit). With that fix the scoped ignore is redundant and would mask the audit on this workflow going forward, so drop it and let xcp-ng#35 be the fix. This branch is back to just the Dependabot config. This reverts commit 4b30ed2. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
|
Ok for me! |
|
I will try to review next wednesday I would say, I'm busy with QA for a few days. |
I would say it's not urgent as @ydirson has some concerns over pushing source code as branches into the SRPMs repos. Even though this predates this PR, I am assuming it will likely change on the implementation side until we have a solution everyone is happy with. |
|
This is ready to be re-reviewed, ater discussing with YannD I've changed the target repository for the source from being the SRPM repo (so split sources for each driver) to pushing to a H&K managed repository meant just for hosting the source branches, to avoid constraints: https://github.com/xcp-ng/driver-sources |
There was a problem hiding this comment.
I still get lots of 404s in the drivers/8.3/source repo directory with the last commit applied.
In the annotations of failed check https://github.com/xcp-ng/hypervisor-dev/actions/runs/27421166386/job/81046832866,
there is an error for the commit fetch for the atlantic-module-alt 8.3 source link, which is part of the 404s.
Is the last commit partly broken?
For commit c19885a, I thought about adding the whole drivers folder instead of every tracked file (-u option), which would be more controlled. You can choose to keep it this way no problem.
ea01c70 to
4b407e3
Compare
Yes, as I was saying in #35 (comment) - I can't run the workflow from the PR directly because it needs a secret passed from github, and they're only passed down to workflows run from main for security reasons. I did make a local run and checked the -alt URLs were well formed, but will re-test after merging and fix-up any remaining issues arising from the github runner context if that's OK with you.
Sure, I've made the change. Thanks for the review! |
LuKP17
left a comment
LuKP17
left a comment
There was a problem hiding this comment.
Looks good on my side, although I'm not as knowledgeable as I'd like with this PR.
Although this was working when run manually, because it would lead the SRPM_REPO_PATH to be equal to the CODE_REPO_PATH in the OOT_DRIVER_IMPORT mode case (and the source branches are actually pushed to the SRPM repo for our out-of-tree drivers for the sake of not having to duplicate each repository), this would not work when run from the github runner because inside its worktree, the srpm and source repository are standalone sub-modules not sharing a git object tree. Signed-off-by: Quentin Casasnovas <quentin.casasnovas@vates.tech>
GitHub runners may use a kernel version that makes rpmspec unhappy when it contains more than one dash separator. Define a known-good kernel version via RPM_OPTS to avoid parse errors, see ff00d02 ("update-drivers-list: fix kernel_version to avoid errors on github runners.") for more details. Signed-off-by: Quentin Casasnovas <quentin.casasnovas@vates.tech>
In such cases, for example with the qlogic driver, all packages would be output on the same line, giving a bogus build directory. Adding the newline allows us to only take the first matching package, and it seems to work for all of our out-of-tree drivers. If this ever changes in the future and the build directory ends up being in a different place, for example on the second package name, we can improve the script to try each until it finds it, but it is not necessary as of now. Signed-off-by: Quentin Casasnovas <quentin.casasnovas@vates.tech>
Otherwise when we run it on hosts with a different timezone offset, we get different author/commiter date, and we lose the idempotency of runs. This happens on github runner hosts versus local hosts in France. Signed-off-by: Quentin Casasnovas <quentin.casasnovas@vates.tech>
… in pr body. Signed-off-by: Quentin Casasnovas <quentin.casasnovas@vates.tech>
Signed-off-by: Quentin Casasnovas <quentin.casasnovas@vates.tech>
We were lacking the drivers/README.md file otherwise. Signed-off-by: Quentin Casasnovas <quentin.casasnovas@vates.tech>
…running scripts depending on it. Signed-off-by: Quentin Casasnovas <quentin.casasnovas@vates.tech>
Signed-off-by: Quentin Casasnovas <quentin.casasnovas@vates.tech>
…e branches. Signed-off-by: Quentin Casasnovas <quentin.casasnovas@vates.tech>
… so that path to modules are correct. Signed-off-by: Quentin Casasnovas <quentin.casasnovas@vates.tech>
The package doesn't include the -alt suffix, so make sure we add it when constructing the source URL. Reported-by: Lucas Pottier <lucas.pottier@vates.tech> Signed-off-by: Quentin Casasnovas <quentin.casasnovas@vates.tech>
… repos. As Nathanaël reported: - repo URL should have a : after .com, like git@github.com:xcpng-rpms - org name misses an - it should be xcp-ng-rpms and not xcpng-rpms Reported-by: Nathanaël Hannebert <nathanael.hannebert@vates.tech> Signed-off-by: Quentin Casasnovas <quentin.casasnovas@vates.tech>
As reported by Nathanaël: - Missmatch: write `/tmp/missing_sources.txt` and test `/tmp/missing-sources.txt` Reported-by: Nathanaël Hannebert <nathanael.hannebert@vates.tech> Signed-off-by: Quentin Casasnovas <quentin.casasnovas@vates.tech>
The approach whereby we were pushing source branches directly into the srpm repos was deemed not wanted, main reason on top of abusing the srpm repo to store non-srpm related objects is that allowing a workflow to have write access on critical repositories like SRPM ones adds unecessary risks to those repos. Instead, to avoid having to duplicate each SRPM repository with an extra source repository, and because the sources are really small for our out-of-tree drivers, let's push all the source branches into a single git repository, [driver-sources](https://github.com/xcp-ng/driver-sources/settings/branches). The branch scheme already allows to differentiate the package name from the branch name, so there won't be any clashes between different packages or ambiguity as to what packages sources are pointing to. As a side-effect bonus, this allows comparing easily a main driver source branch with an alt driver, because they now live in the same repository :) The changes are minimal, and sha1 of source submodules repos have been updated du to the creation of a single shared parent initial commit for all. Signed-off-by: Quentin Casasnovas <quentin.casasnovas@vates.tech>
4b407e3 to
e01d41e
Compare
It's fine, thanks a lot Lucas. main is currently broken, so it's better to try and fix it anyway and I can address any remaining issue only showing up in the github runner context as a subsequent step. |
…ules" The artipacked finding is fixed properly upstream in xcp-ng#35, which sets persist-credentials explicitly on the checkout (what actually silences the audit). With that fix the scoped ignore is redundant and would mask the audit on this workflow going forward, so drop it and let xcp-ng#35 be the fix. This branch is back to just the Dependabot config. This reverts commit 4b30ed2. Signed-off-by: Bruno Verachten <gounthar@gmail.com>
* chore: add Dependabot config for actions, uv, and docker Add weekly Dependabot version updates for GitHub Actions, the two uv-based Python tools (scripts/git-review-rebase and scripts/kabi), and the riscv Docker base images, each with a 7-day cooldown. Driver submodules are intentionally excluded; they are handled by the existing update-driver-submodules workflow. Signed-off-by: Bruno Verachten <gounthar@gmail.com> * chore: scope-ignore zizmor artipacked on update-driver-submodules The update-driver-submodules workflow intentionally persists the GITHUB_TOKEN on checkout so its "Commit and open PR" step can `git push --force origin`. It uploads no artifacts, so the artipacked credential-leak vector (token exposure via uploaded artifacts) does not apply here. Add .github/zizmor.yml scoping the artipacked ignore to that one file, so zizmor passes without weakening the audit for the rest of the workflows. Signed-off-by: Bruno Verachten <gounthar@gmail.com> * Revert "chore: scope-ignore zizmor artipacked on update-driver-submodules" The artipacked finding is fixed properly upstream in #35, which sets persist-credentials explicitly on the checkout (what actually silences the audit). With that fix the scoped ignore is redundant and would mask the audit on this workflow going forward, so drop it and let #35 be the fix. This branch is back to just the Dependabot config. This reverts commit 4b30ed2. Signed-off-by: Bruno Verachten <gounthar@gmail.com> --------- Signed-off-by: Bruno Verachten <gounthar@gmail.com>
3 participants
Looks like the workflow never really worked as expected due to differences in the github worker host versus local runs.
Thanks to @LuKP17 for reporting the issue, which he caught because the drivers README listing supported drivers and their version was stale.
A good run with this branch lead to this PR: https://github.com/xcp-ng/hypervisor-dev/pull/34/changes#diff-7ed8b37d946b3f15d59f978f9047536de899f2949a4041580d4374cee5396e84
It correctly updated all srpm that had changed since last manual runs, as well as their companion source repos. The README changes correctly reflect this.